fix: remove ops.main.main deprecation warning, and avoid warnings in action output #1496
Conversation
Thinking on this more, I think it should be the other one. I'll switch over (and fix whatever the test issue is - maybe 3.8 compatibility since they passed locally) and un-draft. No need to review until then. |
I think there's a 3rd option: remove the warning entirely, and solve the ops.main.main -> ops.main some other way:
Let's brainstorm at the next standup, perhaps? |
I did think about this too. This one in particular is complicated because it's "outside" the charm, so it doesn't get picked up in unit tests and is also complicated to code around because the framework isn't set up yet. I do think that we should have a PR that prevents the general issue of DeprecationWarnings that caused by code in src/charm.py ending up in action output. But we could potentially do that and also give up on this specific warning in favour of other approaches. |
|
Decision from Charm Tech is to remove the warning altogether to keep it simple. And then Charm Tech (whoever works on this) can open 2 or 3 PRs jubilantly (!) to fix this in the "culprit teams". |
tonyandrewmeyer
force-pushed
the
cleaner-warnings-1460
branch
from
2c0d187 to
a589f26
Compare
Yoikes: |
tonyandrewmeyer
changed the title
Ouch. We could still let 2 or 3 teams know with PRs, but yeah, with those numbers, maybe that's not a good approach. I guess just a polite message in Charm Development and leave it at that? Out of interest, are there 312 charms in your dir now, or does this occur more than once per charm? |
More than once. Some are documentation, but that really ought to get fixed too. Some are in libs, so there's some duplication that would remove the number of PRs, but be more annoying to make sure the change is in the right PR. Some repos also have quite a few charms in them, which ups the volume. |
| **__: typing.Any, | ||
| ): | ||
| """Direct the warning to Juju's debug-log, and don't include the code.""" | ||
| logger.warning('%s:%s: %s: %s', filename, lineno, category.__name__, message) |
There was a problem hiding this comment.
what do you think about using warnings.formatwarning(...) to replicate what showwarning does, that is tries to load the source code and show where the warning was issued?
There was a problem hiding this comment.
I don't like unstructured log entries that have newlines in them, and I don't think it's ideal to have code snippets in the Juju debug log. However, we do both of those for exceptions already, so perhaps that doesn't matter. However, I don't think it's that useful for warnings. The default warnings formatter looks like:
/tmp/fw.py:4: DeprecationWarning: Don't call foo(), use the new bar() instead
warnings.warn("Don't call foo(), use the new bar() instead", DeprecationWarning)
That second line is telling me nothing that the first line hasn't already. Maybe the line has something other than the warnings.warn call that provides extra context, but in my experience that's unusual.
That would currently show something like:
unit-my-charm-0: 10:07:20 INFO unit.my-charm/0.juju-log charm.py:42: DeprecationWarning: Don't call foo(), use the new bar() instead
I feel like that's a better form for an unstructured log message.
There was a problem hiding this comment.
I guess that depends on whether the end user looks at single messages are series of messages.
Don't call foo(), use the new bar() instead is great if there's a single call to foo(), and not so much if there are hundreds, or if there's no obvious call, as in a case of being called via getattr(<computed>)().
I can see benefits for both alternatives, I guess.
There was a problem hiding this comment.
Don't call foo(), use the new bar() insteadis great if there's a single call to foo(), and not so much if there are hundreds, or if there's no obvious call, as in a case of being called viagetattr(<computed>)().
But you still have the filename and the line number. You don't have the exact line of code, but assuming you have access to it, it's trivial to open that file and go to that line number. You don't get a full traceback with the default formatter, so you only have the line the warning was pointing at - which is hopefully using a stacklevel that is one up from the actual warning.warn call.
For example:
# charm.py
class MyCharm(ops.CharmBase):
...
def _on_something(self, event):
self.foo() # first one
...
def _on_something_else(self, event):
self.foo() # second one
def foo(self):
warnings.warn("don't foo(), do bar()", DeprecationWarning, stacklevel=2)When _on_something is called, there will be a Juju log entry that points you at charm.py to the line with "# first one", and when on_something_else is called, there will be a Juju log entry that points you at charm.py to the line with "# second one".
If we include the actual code, like the default formatter, then you'd also have in the Juju debug log those actual lines of code, but I don't think this really gains you much (especially when it's a warning aimed at the developer, who will have access to the code) - and it saves having a multi-line log entry.
…ve a deprecation warning.
|
@benhoyt friendly reminder about this PR in case it's slipped off your radar :) |
Background:
warnings.warncall outputs to stderr by default.main()other thanimport ops ... ops.main()__main__module.main()or execute thecharm.pyfile, so they will not trigger this warning. That means that we're relying on integration tests.The above means that, currently, the deprecation warning is included in the action output - this warning is intended for the developer of the charm, not the user of the charm, so we want to change this.
Firstly, the
ops.main.maindeprecation warning is removed. It's too awkward to have a warning that gets emitted before the framework is set up. Instead, we'll rely oncharmcraft analyseand static checks.However, while that will solve the specific issue in #1460, we do also want to avoid any future leakage of warnings into action output if there are other warnings emitted that originate in the
__main__model.To that, rather than rely on Juju capturing the warning via stderr, we use the logging package to capture all warnings and redirect them to logging calls. These are then directed to Juju's debug-log via the existing mechanism. While we are doing this, we minimise the output to include references to the filename and line, but not the actual code itself (which may be sensitive).
When running (Scenario) tests, we ensure that the warning and logging systems use their default behaviour, so that pytest and other tools can work as expected.
Fixes #1460